Skip to content

Minimize lock usage in InternalLoggerRegistry#3418

Merged
vy merged 4 commits into2.24.xfrom
fix/3399_create_logger_no_lock
Feb 6, 2025
Merged

Minimize lock usage in InternalLoggerRegistry#3418
vy merged 4 commits into2.24.xfrom
fix/3399_create_logger_no_lock

Conversation

@ppkarwasz
Copy link
Contributor

@ppkarwasz ppkarwasz commented Jan 28, 2025

It has been reported that holding a lock on InternalLoggerRegistry during the creation of a logger can cause performance loss and deadlocks. The logger constructor can trigger property lookups and other pluggable operations, we don't entirely control. The fix to #3252 only covered one of these cases.

This change moves the instantiation of new Loggers outside the write lock. While in some cases, this will cause multiple instantiations of loggers with the same parameters, all such loggers are functionally equivalent. On the other hand, the change allows the creation of different loggers in parallel.

Closes #3399

@ppkarwasz
Minimize lock usage in InternalLoggerRegistry
98189ee 
It has been reported that holding a lock on `InternalLoggerRegistry` during the creation of a logger can cause performance loss and deadlocks. The logger constructor can trigger property lookups and other pluggable operations, we don't entirely control. The fix to #3252 only covered one of these cases.

This change moves the instantiation of new `Logger`s outside the write lock. While in some cases, this will cause multiple instantiations of loggers with the same parameters, all such loggers are functionally equivalent. On the other hand, the change allows the creation of different loggers in parallel.

Closes #3399
@ppkarwasz ppkarwasz requested a review from vy January 28, 2025 07:44
@ppkarwasz ppkarwasz mentioned this pull request Jan 28, 2025
Closed
vy
vy requested changes Jan 28, 2025
View reviewed changes
@tristantarrant tristantarrant mentioned this pull request Jan 31, 2025
Closed
ppkarwasz
ppkarwasz commented Feb 4, 2025
View reviewed changes
Copy link
Contributor Author

@ppkarwasz ppkarwasz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vy, looks good to me!

@vy
Copy link
Member

vy commented Feb 4, 2025

Tried implementing a test1 where ILR read and write methods are stressed using multiple threads – see the attachment. Even after several different configuration attempts – increased test duration, logger name cardinality, played around with ro-vs-rw operation ratios, etc. – had no luck.

1 InternalLoggerRegistryConcurrencyTest.java.txt

@vy vy merged commit 3ffaf76 into 2.24.x Feb 6, 2025
7 checks passed
@vy vy deleted the fix/3399_create_logger_no_lock branch February 6, 2025 14:18
@vy vy self-assigned this Feb 6, 2025
@ppkarwasz
Copy link
Contributor Author

ppkarwasz commented Feb 6, 2025

@vy,

Thanks for merging this. Could you cherry-pick it to 2.x and main too? 🥺

vy added a commit that referenced this pull request Feb 6, 2025
@ppkarwasz @vy
Minimize lock usage in InternalLoggerRegistry (#3418)
a4dfb37 
Co-authored-by: Volkan Yazıcı <[email protected]>
@vy
Copy link
Member

vy commented Feb 6, 2025

@ppkarwasz, ported to 2.x. main needs some work, since #3199 needs to be ported too.

@ppkarwasz ppkarwasz mentioned this pull request Jan 13, 2025
Open
27 tasks
@ppkarwasz
Copy link
Contributor Author

ppkarwasz commented Feb 6, 2025

Thanks, I added it to the backlog #3161

@ppkarwasz ppkarwasz mentioned this pull request May 29, 2025
Merged
@jvz
Copy link
Member

jvz commented Jul 31, 2025

I don't see anywhere in main this even applies. We don't have an InternalLoggerRegistry there and rely on LoggerRegistry from log4j-api.

@ppkarwasz
Copy link
Contributor Author

ppkarwasz commented Jul 31, 2025

I think the entire InternalLoggerRegistry should be ported to main.

We introduced it so log4j-core does not depend on that helper class from log4j-api, which limits problems if the versions of log4j-core and log4j-api are mismatched.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vy vy vy requested changes

+1 more reviewer

@tristantarrant tristantarrant tristantarrant left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@vy vy

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ppkarwasz @vy @jvz @tristantarrant